pr05 Typescript Migration #5: Migrate client/common folder #3565
Conversation
…ript parser on ts files
…omponents, remove react/require-default-props rule
| import styled from 'styled-components'; | ||
| import { Link } from 'react-router-dom'; | ||
|
|
||
| import { Link, LinkProps } from 'react-router-dom'; | ||
| import { remSize, prop } from '../theme'; | ||
|
|
||
| const kinds = { |
There was a problem hiding this comment.
think this could also be a typescript enum
|
|
||
| const displays = { | ||
| block: 'block', | ||
| inline: 'inline' | ||
| } as const; |
| const buttonTypes = { | ||
| button: 'button', | ||
| submit: 'submit' | ||
| } as const; |
| display: Display; | ||
| }; | ||
|
|
||
| type SharedButtonProps = { |
There was a problem hiding this comment.
can this be an interface instead? same for all other similar cases with the Props
There was a problem hiding this comment.
| /** | ||
| * Specifying an href will use an <a> to link to the URL | ||
| */ | ||
| href?: string | null; |
There was a problem hiding this comment.
feels like if href can be undefined then we should avoid also letting it be null?
| * Allows for IconButton to pass `focusable="false"` as a prop for SVGs. | ||
| * See @types/react > interface SVGAttributes<T> extends AriaAttributes, DOMAttributes<T> | ||
| */ | ||
| focusable?: boolean | 'true' | 'false'; |
There was a problem hiding this comment.
can we have it just be the boolean type? if the parent that is using this component has the information stored as a string we should have it be the parents' responsibility to cast it to a boolean type before it's passed in
| @@ -181,69 +249,7 @@ const Button = ({ | |||
| ); | |||
| }; | |||
|
|
|||
| Button.defaultProps = { | |||
There was a problem hiding this comment.
it's so great that we can get rid of all of this extra code at the bottom of the React files now haha, thanks for doing this conversion!!
| */ | ||
| export type ButtonOrLinkProps = { |
There was a problem hiding this comment.
can this be an interface?
| ButtonProps, | ||
| 'iconBefore' | 'display' | 'focusable' | ||
| > & { | ||
| icon?: ComponentType<{ 'aria-label'?: string }> | null; |
There was a problem hiding this comment.
same here maybe prefer to remove the | null and let the nullish cases all be undefined
| * @param value - The current value to track. | ||
| * @returns The previous value before the current render, or undefined if none. | ||
| */ | ||
| export default function usePrevious(value: number): number | undefined { |
There was a problem hiding this comment.
is there any way we can avoid having it be typed to numbers specifically? Maybe this is a rare case where it makes sense to use any or unknown? that way the hook can be used for more general cases in the future
pr05 Typescript Migration 5: Migrate the client/commonfolder
Context:
Rebuild of https://github.com/processing/p5.js-web-editor/pull/3564/files for cleaning commits
Migrate as much of the client/common folder with the following steps:
git mv someComponent.jsx someComponent.tsx. If possible, commit without theno-verifyflag, otherwise withno-verify.Changes:
parser: "babel"settingisMac()utility from pr05 Typescript #3: Migrate client/utils folder #3553DocumentKeyDown-- not used anywhere anymore, likely carry-over/legacy for class componentsDidn't migrate the storebook files as I wasn't able to run storybook locally to verify against silent regression
Notes:
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123